Skip to content

Fix missing return in v1v2_observable_df noise distribution merge#502

Merged
dweindl merged 2 commits into
PEtab-dev:mainfrom
dweindl:fix/v1v2-observable-df-noise-distribution
Jul 9, 2026
Merged

Fix missing return in v1v2_observable_df noise distribution merge#502
dweindl merged 2 commits into
PEtab-dev:mainfrom
dweindl:fix/v1v2-observable-df-noise-distribution

Conversation

@dweindl

@dweindl dweindl commented Jul 9, 2026

Copy link
Copy Markdown
Member

update_noise_dist lacked a return statement, so the merged noiseDistribution column was always NaN after petab1to2 conversion.

update_noise_dist lacked a return statement, so the merged
noiseDistribution column was always NaN after petab1to2 conversion.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@codecov-commenter

codecov-commenter commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.12%. Comparing base (2bb8830) to head (0e0eee3).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #502      +/-   ##
==========================================
+ Coverage   75.09%   75.12%   +0.03%     
==========================================
  Files          64       64              
  Lines        7160     7161       +1     
  Branches     1267     1267              
==========================================
+ Hits         5377     5380       +3     
+ Misses       1293     1291       -2     
  Partials      490      490              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dilpath dilpath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Can rename update_noise_dist to e.g. get_noise_dist since update implies inplace to me.

e.g. https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.DataFrame.update.html

@dweindl dweindl merged commit 1b8599d into PEtab-dev:main Jul 9, 2026
7 checks passed
@dweindl dweindl deleted the fix/v1v2-observable-df-noise-distribution branch July 9, 2026 20:00
@FFroehlich

Copy link
Copy Markdown
Collaborator

🚀

FFroehlich added a commit to AMICI-dev/AMICI that referenced this pull request Jul 9, 2026
The `update_noise_dist` missing-return bug in petab.v2.petab1to2 (which
silently reverted every upgraded observable's noiseDistribution to
`normal`, corrupting iy_trafos/chi2 for non-linear observable
transformations) has been fixed upstream in
PEtab-dev/libpetab-python#502.

Remove the `_fix_petab1to2_noise_distribution_bug` shim and bump the v1
petab-suite CI job's libpetab pin from 44c8062 to 1b8599dd (the #502
merge commit) so the fix is present. Case 0016 now passes via the
upstream conversion; case 0007's xfail (inherent log10-normal ->
log-normal substitution) and its warning filter remain.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
FFroehlich added a commit to AMICI-dev/AMICI that referenced this pull request Jul 10, 2026
…ution, and differentiability (#3207)

* Fix CI failures in JAX PEtab v1/v2 backend

Fixes a cluster of bugs surfaced by CI once the v1->v2 upgrade path
(from the two separate PRs merged into this branch) started actually
reaching previously-unreachable code:

* get_simulation_conditions_v2: a dynamic period may reference
  multiple condition ids (e.g. a synthetic preequilibration-indicator
  condition alongside the real experiment condition). Measurements are
  only ever queried by experiment id, so multiple condition-id rows
  per experiment produced duplicate/misaligned measurement arrays,
  causing vmap batch-size mismatches (`vmap got inconsistent sizes`).
  Collapse to one row per experiment.

* add_default_experiment_names_to_v2_problem: read condition ids from
  condition table elements instead of the long-format `condition_df`,
  which contributes zero rows for a condition with no changes (e.g.
  the default condition, or any no-op condition) -- exactly the
  "Experiment has no dynamic period with a condition id" case.

* _build_simulation_df_v2: the synthetic default experiment id was
  overwritten with NaN before being reused to query
  observableParameters/noiseParameters from the measurement table,
  silently matching nothing.

* _get_parameter_mappings: a condition table's target value can be a
  reference to another PEtab parameter id (not just a numeric
  literal), which crashed trying to cast the symbol straight to a jax
  array. Resolve it the same way `_map_experiment_model_parameter_value`
  already resolves other parameter references.

* import_petab_problem (legacy v1 path): snapshot the pristine v1
  problem before SBML/PySB compilation mutates it in place, so the
  later v1->v2 upgrade doesn't serialize an already-mutated (and
  potentially v1-lint-failing) problem.

* pytest.ini: ignore the benign, documented petab1to2 warning when
  falling back from a v1-only noise distribution (log10-normal) to
  log-normal for v2 -- was being promoted to a hard error by this
  repo's `filterwarnings = error` policy, exactly when the v1->v2
  upgrade path first became reachable.

* ExampleJaxPEtab.ipynb / test_petab_suite.py: two consumers still
  expected `dynamic_conditions` to hold bare condition-id strings;
  it now holds tuples (to support multi-condition periods). Updated to
  match.

Brings the official PEtab v1/v2 test suite (jax=True) from 28 failing
down to 18 (case 0007's chi2 mismatch is a likely-inherent consequence
of the log10-normal->log-normal fallback; the remaining ~8 cases
cluster around condition-table-driven state reinitialisation and are
tracked separately). jax=False path re-verified unaffected.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* Ignore another benign petab1to2 warning-turned-error

Fixes a regression: petab1to2's "Parameter scales are not supported
in PEtab v2" warning (emitted whenever a v1 problem uses non-linear
parameterScale, e.g. the lotka_volterra test fixture) was being
promoted to a hard error by this repo's `filterwarnings = error`
policy, breaking test_preequilibration_failure/test_serialisation.

Verified benign via direct SUNDIALS simulation of the same converted
v2 problem (bypassing JAX) for petab test suite cases 0019/0020, which
also trip this warning: llh matches the ground truth solution exactly,
confirming the dropped parameterScale is purely estimation-scale
metadata that doesn't affect simulation values (petab v1's
nominalValue is always stored in linear units).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* Fix state reinitialisation lookup and petab1to2 noise-distribution bug in JAX PEtab backend

Resolves the remaining 18 official PEtab test suite (jax=True) failures
(cases 0007, 0010, 0011, 0013, 0016-0020, both formats):

* JAXProblem._state_needs_reinitialisation/_state_reinitialisation_value/
  load_reinitialisation looked up species-level condition-table overrides
  via the old wide-format `condition_df.loc[condition, state_id]`, but the
  current petab.v2 API returns `condition_df` in long format
  (conditionId/targetId/targetValue columns), so the lookup always missed
  and every such override silently fell back to the SBML default. Rewired
  to reuse `_parameter_mappings["targets_map"]` (built from `c.changes`),
  which parameter-target lookups already relied on correctly. Fixes cases
  0010, 0011, 0013, 0017, 0018, 0019, 0020.

* Traced case 0007/0016's chi2-only mismatches (LLH and simulated values
  already matched) to an upstream libpetab-python bug: petab1to2's
  `update_noise_dist` computes the merged v1->v2 `noiseDistribution` (e.g.
  folding `observableTransformation=log` into `log-normal`) but never
  returns it, so every upgraded observable silently reverts to `normal`,
  discarding any log/log10 transform. This corrupted `iy_trafos` (and thus
  chi2) even though AMICI's own log-likelihood code generation is
  unaffected, since it's derived from the pristine v1 problem directly.
  Added a workaround in `import_petab_problem` that recomputes the correct
  value from the pristine v1 problem and patches it onto the upgraded v2
  observables. Fixes case 0016 outright.

* With the above fixed, case 0007 has one genuinely inherent residual:
  PEtab v2 has no `log10-normal` distribution, so `log-normal` is
  substituted (with a warning); recomputing chi2 with log10 in place of
  log reproduces the expected ground-truth value exactly, confirming this
  is a real v1->v2 upgrade limitation, not an AMICI bug. Marked as an
  explicit, documented `pytest.xfail` rather than silently skipped.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* Resolve JAX PEtab reinitialisation values live so gradients flow

The previous commit resolved condition-table state initial values through
`_parameter_mappings["targets_map"]`, which `_get_parameter_mappings` builds
once at construction time. For a value referencing an estimated parameter,
that captured `self.parameters[i]` as a constant in a separate (frozen)
pytree leaf. Consequences:

* `update_parameters(...)` only replaces `.parameters`, never the cached
  `targets_map`, so the initial value stayed pinned at the nominal parameter
  value -- re-simulating after an update silently ignored it.
* Differentiating via the documented `eqx.filter_grad(run_simulations)`
  idiom put the sensitivity into `grad._parameter_mappings[...]` rather than
  `grad.parameters`, so `grad.parameters` (what callers read) was 0 for any
  parameter used as an initial value.

Verified on petab test suite case 0020 (initial_A estimated, entering the
likelihood only through A(0)): pre-fix, updating initial_A left llh
unchanged and grad.parameters[initial_A] was 0 while -8.70 leaked into the
cache.

Fix: resolve the reinitialisation value live from the raw condition-table
change (`_condition_reinit_target_value` + `_resolve_condition_target_value`)
inside `_state_reinitialisation_value`, which runs within the traced region
via `_prepare_experiments`. Reading `self.parameters` there keeps the value
a function of the current parameters. After the fix, autodiff matches
central finite differences to ~1e-10 with zero gradient leaking into the
cache, for estimated initial values (cases 0020, 0019), parameter-referenced
initial values (case 0013) and ordinary rate parameters alike.

Adds `test_condition_table_initial_value_is_differentiable` (the petab test
suite skips derivative checks for jax, so this bug was uncaught).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Resolve condition-table parameter overrides live too

Same construction-time freezing bug as the preceding commit, on the
parameter-mapping path: `_map_experiment_model_parameter_value` read the
override value from `_parameter_mappings["targets_map"]`, which caches
`self.parameters[i]` at construction time. So a model parameter mapped by
the condition table to an estimated parameter -- the standard PEtab pattern
for condition-specific estimated parameters -- was frozen at the nominal
value: `update_parameters` had no effect and its gradient leaked into the
cache instead of `grad.parameters`.

Verified: a condition setting model parameter `k1` to estimated `k1_c0`
previously left llh unchanged under `update_parameters(k1_c0)` with
`grad.parameters[k1_c0] == 0` (and -0.40 leaking into the cache); after the
fix, forward responds and autodiff matches central finite differences.

Fix: build the override lookup from the raw condition-table changes and
resolve it live via `_resolve_condition_target_value` (which reads the live
`self.parameters` inside the traced region), mirroring the reinitialisation
fix. Adds `test_condition_table_parameter_override_is_differentiable`.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Apply suggestions from code review

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* Drop petab1to2 noise-distribution workaround; rely on upstream fix

The `update_noise_dist` missing-return bug in petab.v2.petab1to2 (which
silently reverted every upgraded observable's noiseDistribution to
`normal`, corrupting iy_trafos/chi2 for non-linear observable
transformations) has been fixed upstream in
PEtab-dev/libpetab-python#502.

Remove the `_fix_petab1to2_noise_distribution_bug` shim and bump the v1
petab-suite CI job's libpetab pin from 44c8062 to 1b8599dd (the #502
merge commit) so the fix is present. Case 0016 now passes via the
upstream conversion; case 0007's xfail (inherent log10-normal ->
log-normal substitution) and its warning filter remain.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Revert unnecessary _petab_import.py changes

The pristine-v1-problem snapshot (deepcopy before compilation, used for
the v1->v2 upgrade instead of the possibly-mutated problem) is not needed:
the full v1.0.0 jax petab test suite -- including the placeholder cases
(0003/0004/0005/0009) that trigger `_workaround_observable_parameters`'s
in-place mutation -- passes upgrading `petab_problem` directly, as on main.
Restore the file to match main so this PR leaves it untouched.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Fix _build_simulation_df_v2: consistent masking, drop fragile sc[0] lookup

Two issues in the PEtab v2 simulation-DataFrame builder:

* Length inconsistency (reviewer comment): the DataFrame mixed
  masked/valid-only arrays (obs, t[mask], y[mask]) with unmasked lengths
  (`len(t)`, `index=_petab_measurement_indices[ic, :]`). Since
  `_get_measurements` pads every experiment to a common length, any
  problem whose experiments have differing numbers of timepoints would
  raise `ValueError: arrays must all be same length` or leak
  padded/duplicated indices. Apply the per-experiment `_ts_masks[ic]`
  mask consistently to the index and every column.

* Fragile zero-indexing: the experiment id was recovered by reverse-
  mapping `dyn_conditions`' first condition id (`sc[0]`) through
  `_conditions_to_experiment_map`. `run_simulations` already builds these
  per experiment, so thread the experiment ids through directly and drop
  both the `sc[0]` lookup and the now-unused `_conditions_to_experiment_map`.

No PEtab test-suite case exercises ragged (differing-timepoint)
experiments, so add `test_petab_simulate_ragged_experiments` as a
regression test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants